Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Polyfill the other secp256k1 libs #367

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Polyfill the other secp256k1 libs #367

wants to merge 6 commits into from

Conversation

Duddino
Copy link
Member

@Duddino Duddino commented May 13, 2024 •
edited
Loading

Abstract

Through various dependencies we are currently using 3 different secp256k1 libraries.
The aim of this PR is to remove these dependencies in favor of @noble/secp256k1.

tiny-secp256k1

A noble based library was already developed by bitcoiner lab

secp256k1

A replacement for this library was developed in-house here.
The original tests are being used to test this version of the library.

Why switch to a single secp256k1?

Using a single library allows us to reduce the bundle size, and the potential vulnerabilities.
The other libraries have a larger amount of dependencies, like elliptic.js and bn.js, and as far as I can tell have not been audited.

This brings the main bundle size down from 361KB to 325 KB(gzipped)

Testing

  • Test that hd keys work as expected
  • Test transactions

When testing is done, secp256k1-noble will be released on npm (With a better name)

@netlify Netlify
Copy link

netlify bot commented May 13, 2024 •
edited
Loading

✅ Deploy Preview for cheery-moxie-4f1121 ready!

Name Link
🔨 Latest commit 763ce92
🔍 Latest deploy log https://app.netlify.com/sites/cheery-moxie-4f1121/deploys/66c5fd950ade980008002e4e
😎 Deploy Preview https://deploy-preview-367--cheery-moxie-4f1121.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Duddino Duddino added the Performance An improvement and/or suggestion regarding performance label May 13, 2024
@Duddino Duddino self-assigned this May 13, 2024
JSKitty
JSKitty requested changes Jun 7, 2024
View reviewed changes
Copy link
Member

@JSKitty JSKitty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting this error when attempting to create a new HD wallet:
image

Uncaught (in promise) TypeError: secp256k1.privateKeyVerify is not a function
    set hdkey.js:36
    fromMasterSeed hdkey.js:193
    HdMasterKey masterkey.js:114
    f parsed_secret.js:77

JSKitty
JSKitty approved these changes Aug 15, 2024
View reviewed changes
Copy link
Member

@JSKitty JSKitty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK a50d16d

Tested both Import and Creation of HD wallets.
Received from Shield, sent to self, then sent to an external wallet - no issues.
Generated many addresses, checked the xpub on the explorer.

A-OK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JSKitty JSKitty JSKitty approved these changes

Assignees

@Duddino Duddino

Labels
Performance An improvement and/or suggestion regarding performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Duddino @JSKitty